-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved AssetImage Docs #78173
Improved AssetImage Docs #78173
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@@ -116,7 +116,11 @@ const double _kLowDprLimit = 2.0; | |||
/// ```dart | |||
/// AssetImage('icons/heart.png') | |||
/// ``` | |||
/// See also: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line before paragraph
/// | ||
/// * [loading images](https://flutter.dev/docs/development/ui/assets-and-images#loading-images-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start each bullet with a captital letter
/// | ||
/// * [loading images](https://flutter.dev/docs/development/ui/assets-and-images#loading-images-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That link doesn't actually show how to use an AssetImage to get the asset, as far as i can tell. It shows how to use an AssetImage with an Image widget but that's already explained in the "See also" link at the bottom of this section.
What #78088 is asking for is for sample code showing how to actually get a ui.Image
object out of an AssetImage
.
@Swayam221 Do you still have plans to come back to this PR? |
@goderbauer Yes sir! I will submit a PR after making some amends by tonight . |
@Hixie Sir, pointed it to the image provider class, will this do? |
@@ -116,7 +116,9 @@ const double _kLowDprLimit = 2.0; | |||
/// ```dart | |||
/// AssetImage('icons/heart.png') | |||
/// ``` | |||
/// | |||
/// | |||
/// To understand how an [Image] object is made out of an [AssetImage], refer [ImageProvider] and [AssetBundleImageProvider]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to adjust the Engilsh here for grammar and to fit the Flutter style, something more like:
/// For an example of resolving an [AssetImage] to obtain a [dart:ui.Image] object,
/// refer to the sample code in the documentation for [ImageProvider].
@Swayam221 The analyzer check is reporting some unnecessary whitespace. Can you fix that up please? |
@goderbauer Fixed It Sir! |
I recommend retriggering the tests, looks like you may have last updated the PR when the tree was broken in some way. |
@Hixie looks like tree is still broken? |
Yeah, but |
@Swayam221 Do you still have plans to come back to this PR? |
@goderbauer Yes sir. Sorry for the delays, I was involved with college work and was trying to apply for GSoC. I will try to submit a PR asap 😅. |
@goderbauer if the load() method in ImageProvider is responsible for fetching an image, will it be okay if I make it point to https://api.flutter.dev/flutter/painting/ImageProvider/load.html? |
In the docs for ImageProvider there's a section labeled "Using an ImageProvider", which has sample code that explains how to use an ImageProvider to get a |
@Hixie Does the current version of this PR than answer your question? |
Since the proposed sentence seemed unclear to you, it probably needs extra clarification. The easiest solution is probably just to put the same code here, updated to work with AssetImage specifically. |
This is looking pretty good now. Can you rebase it to the latest master so all the checks will pass? Thanks. |
@goderbauer @Hixie Review Requested. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR aims at improving the api docs for AssetImage.
Fixes #78088
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.